Skip to content

eliminate internal tracking of EOF#6015

Open
rnewson wants to merge 1 commit into
mainfrom
no-eof
Open

eliminate internal tracking of EOF#6015
rnewson wants to merge 1 commit into
mainfrom
no-eof

Conversation

@rnewson
Copy link
Copy Markdown
Member

@rnewson rnewson commented May 26, 2026

Overview

Manually tracking is error-prone. e.g, file:write might return ok but not have written all bytes due to buffering. couch_file advances the eof anyway, so the results of subsequent writes before a datasync will return the wrong position. calling file:position/2 is very cheap, so just do that when we're writing.

Testing recommendations

Covered by eunit tests.

Related Issues or Pull Requests

N/A

Checklist

  • This is my own work, I did not use AI, LLM's or similar technology
  • Code is written and works correctly
  • Changes are covered by tests
  • Any new configurable parameters are documented in rel/overlay/etc/default.ini
  • Documentation changes were made in the src/docs folder
  • Documentation changes were backported (separated PR) to affected branches

@rnewson
Copy link
Copy Markdown
Member Author

rnewson commented May 26, 2026

noting this might make the read_beyond_eof handling code unreachable (iirc we added that when we speculated sources of eof tracking being wrong in ways we couldn't reproduce).

@rnewson rnewson requested a review from nickva May 26, 2026 13:00
nickva
nickva reviewed May 26, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@nickva nickva left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this change, it simplifies things quite a bit. We'll have to check a bit more thoroughly but a preliminary quick bench shows the extra syscall costs about 3-5 microseconds

macos/intel

>element(1,timer:tc(fun() -> [file:position(Fd, eof) || I <- lists:seq(1,1000000)], ok end))/1000000.
3.739081

debian trixie/intel

f(Fd), {ok, Fd} = file:open("/var/tmp/f.txt", [binary, append, create,raw]).
(dbcore@db2.relengtest001.cloudant.net)5> element(1,timer:tc(fun() -> [file:position(Fd, eof) || I <- lists:seq(1,1000000)], ok end))/1000000.
4.243807

Another thing to keep an eye on is if this could acquire any extra locks or affect concurrency. Say would 10k couch_file doing an extra lseek call hit any possible bottleneck in the os layer (I would wager, not but worth thinking about)

@rnewson rnewson force-pushed the no-eof branch 4 times, most recently from e2300c5 to 4c3fa2e Compare May 28, 2026 10:15
@rnewson rnewson marked this pull request as ready for review May 28, 2026 10:15
nickva
nickva requested changes May 28, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@nickva nickva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 with one minor optimization to reduce syscall count for pread batches

Comment thread src/couch/src/couch_file.erl Outdated
@rnewson rnewson force-pushed the no-eof branch 2 times, most recently from 39b1223 to bc656f7 Compare May 28, 2026 18:59
nickva
nickva approved these changes May 28, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@nickva nickva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@nickva
Copy link
Copy Markdown
Contributor

nickva commented May 28, 2026

I ran a few k6-couch benchmark runs against main

BENCH_DOCS=100000 BENCH_DURATION=3m BENCH_SCENARIOS=doc_get,doc_insert  k6 run k6_couchdb.js
...
running (3m46.0s), 00000/01000 VUs, 198002 complete and 0 interrupted iterations
doc_get    ✓ [======================================] 0000/0500 VUs  3m0s  1000.00 iters/s
doc_insert ✓ [======================================] 0000/0500 VUs  3m0s  100.00 iters/s

(100k docs, doc get and doc insert for 3 minute)

Main:

       { name:doc_get }.............: avg=1.68ms min=1.06ms med=1.48ms max=116.65ms p(90)=1.8ms  p(95)=1.93ms
      { name:doc_insert }..........: avg=3.95ms min=2.62ms med=3.5ms  max=151.07ms p(90)=4.24ms p(95)=4.57ms

      { name:doc_get }.............: avg=2.03ms min=1.06ms med=1.48ms max=149.24ms p(90)=1.82ms p(95)=1.98ms
      { name:doc_insert }..........: avg=4.6ms  min=2.6ms  med=3.51ms max=170.73ms p(90)=4.29ms p(95)=4.74ms

      { name:doc_get }.............: avg=2.12ms min=1.06ms med=1.49ms max=157.79ms p(90)=1.81ms p(95)=1.99ms
      { name:doc_insert }..........: avg=4.58ms min=2.59ms med=3.51ms max=164.38ms p(90)=4.28ms p(95)=4.68ms

No Eof (PR)

      { name:doc_get }.............: avg=2.25ms min=1.04ms med=1.56ms max=162.39ms p(90)=1.98ms p(95)=2.23ms
      { name:doc_insert }..........: avg=5.15ms min=2.67ms med=3.97ms max=193.4ms  p(90)=5.11ms p(95)=5.93ms

      { name:doc_get }.............: avg=1.85ms min=1.07ms med=1.52ms max=176.15ms p(90)=1.89ms p(95)=2.05ms
      { name:doc_insert }..........: avg=4.49ms min=2.73ms med=3.85ms max=210.74ms p(90)=4.8ms  p(95)=5.29ms

      { name:doc_get }.............: avg=1.77ms min=1.07ms med=1.53ms max=148.22ms p(90)=1.9ms  p(95)=2.07ms
      { name:doc_insert }..........: avg=4.4ms  min=2.76ms med=3.88ms max=123.9ms  p(90)=4.79ms p(95)=5.31ms

The difference is there but is tiny:

Medians for doc_get
main: 1.48, 1.48,1.49
pr: 1.56,1.52,1.53

Medians for doc_insert:
main: 3.5, 3.51, 3.51
pr: 3.97, 3.85, 3.88

So maybe 0.1-0.5 msec difference

P90 for doc_get:
main: 1.8, 1.82, 1.81
pr: 1.98, 1.89, 1.9

P90 for doc_insert:
main: 4.24, 4.29, 4.28
pr: 5.11, 4.8, 4.79

About the same for reads and <1msec difference for writes

@rnewson rnewson force-pushed the no-eof branch 2 times, most recently from 24e7873 to bd3afbf Compare May 29, 2026 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickva nickva nickva approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rnewson @nickva